Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

Call commands defined by extensions #119

Merged
merged 11 commits into from
Sep 23, 2019
Merged

Call commands defined by extensions #119

merged 11 commits into from
Sep 23, 2019

Conversation

makandre
Copy link
Contributor

@makandre makandre commented Sep 13, 2019 •
edited
Loading

@makandre
remove hardcode appsody
00931ce 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
extensions api route
6e97062 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
call extensions api
d79dd91 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
call command
3fea17d 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
typo
121c133 
Signed-off-by: Andrew Mak <[email protected]>
@makandre makandre changed the title Ext cmd Call commands defined by extensions Sep 16, 2019
@makandre
Copy link
Contributor Author

@liamchampton or @tobespc could you review this?

liamchampton
liamchampton suggested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@liamchampton liamchampton left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but have left a few comments for changes. Please can you also specify how these changes have been tested in the PR comment box as i am still working on getting tests into Jenkins.

actions/project.go
@@ -52,9 +53,9 @@ func DownloadTemplate(c *cli.Context) {

// Remove invalid characters from the string we will use
// as the project name in the template.
r := regexp.MustCompile("[^a-zA-Z0-9._-]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was actually the auto-linter that changed this :)

actions/project.go Outdated Show resolved Hide resolved
actions/project.go Outdated Show resolved Hide resolved
utils/extensions.go
}
log.Printf("Please wait while the Appsody project is initialized... %s \n", output.String())
log.Printf("Please wait while the project is initialized... %s", output.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fmt.Printf() as this is currently going to stderr by default and not stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I needed to revert this back to log. Because stdout needed to be json only because it is parsed by IDE

@makandre
address a couple of comments
1890960 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
rename function
4b29510 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
Copy link
Contributor Author

@liamchampton I tested this change with https://github.com/makandre/codewind-appsody-extension/blob/ext_cmd/codewind.yaml

On mac, I built the installer and copied it to the location where VSCode plugin puts the installer (alongside appsody CLI). Then I invoked it on an appsody project:

andrews-mbp:codewind-installer andrewmak$ /var/folders/c8/12yjw3yn40gcmw3mpzv0sj6c0000gn/T/codewind-installer project /opt/Repos/Tempest/codewind/codewind-workspace/knnode/
Please wait while the project is initialized... 2019/09/18 09:50:37 *
*
*

A new CLI update is available.
Please go to https://github.com/appsody/appsody/releases/latest and update from 0.4.3 --> 0.4.4.

*
*
*
Setting up the development environment
Running command: docker pull appsody/nodejs-express:0.2 
Running command: docker run --rm --entrypoint /bin/bash appsody/nodejs-express:0.2 -c find /project -type f -name .appsody-init.sh 
Successfully initialized Appsody project

{"status":"success","projectPath":"/opt/Repos/Tempest/codewind/codewind-workspace/knnode/","result":{"language":"nodejs","projectType":"appsodyExtension"}}

From that I'm able to see the appsody init was called and now the projectType returned is also correct.

@sghung did similar testing on Windows

@makandre
return unknown
0ad6861 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
Copy link
Contributor Author

Also, for error case (PFE not running):

andrews-mbp:codewind-installer andrewmak$ /var/folders/c8/12yjw3yn40gcmw3mpzv0sj6c0000gn/T/codewind-installer project /opt/Repos/Tempest/codewind/codewind-workspace/knnode/
2019/09/18 10:14:18 There was a problem retrieving extensions data
{"status":"failed","projectPath":"/opt/Repos/Tempest/codewind/codewind-workspace/knnode/","result":"Get http:///api/v1/extensions: http: no Host in request URL"}

@makandre
revert a change
c59beb7 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
fix for python detection
8c6072a 
Signed-off-by: Andrew Mak <[email protected]>
@makandre
Copy link
Contributor Author

Ported part of fix #126 to this PR for master

@makandre
Copy link
Contributor Author

@liamchampton any other changes required for this PR?

@makandre
prevent path traversal
3eaaf17 
Signed-off-by: Andrew Mak <[email protected]>
liamchampton
liamchampton approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@liamchampton liamchampton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. @mattcolegate would you mind having a look over this as you were the last to edit this?

mattcolegate
mattcolegate approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@mattcolegate mattcolegate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sghung
sghung approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@sghung sghung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thank you

@sghung sghung merged commit 58b800d into eclipse-archived:master Sep 23, 2019
@makandre makandre deleted the ext_cmd branch September 23, 2019 16:41
@makandre makandre mentioned this pull request Sep 26, 2019
Closed
@makandre makandre mentioned this pull request Apr 28, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liamchampton liamchampton liamchampton approved these changes

@mattcolegate mattcolegate mattcolegate approved these changes

@sghung sghung sghung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Codewind no longer auto-detect appsody project
4 participants
@makandre @sghung @mattcolegate @liamchampton